-
Notifications
You must be signed in to change notification settings - Fork 22
fix: use path normalization to fix win32 issues #108
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
Conversation
|
@pschiel Can you confirm this was tested, how you tested it and how I can reproduce? |
I fixed only issues that were not working, I reproduced all of the errors. test suite ( other than these, just plenty of usage since ~2 months. backslashes are the biggest problem, relative paths and cross-drive paths. i had a testing command that checked some of these (requires some structure/files setup). when working in a windows env, it's inevitable that mixed paths come from anywhere, all of them should work: |
|
if you require more info / steps to reproduce for a certain tool or so, let me know. some others were issue always same - string matches, contains() checks etc can only work if paths are internally normalized |
|
you might want to look this is a symlink in the repo, which doesn't work in windows, hence the change into a regular file with export. (I noticed a check failing due to this file in your CI) |
|
Interesting that the windows runner tests are failing but passing locally for you |
these are just e2e tests which do a few clicks and that's it - I don't think the unit+integration tests run in a win32 runner. |
|
Since last opencode merge, |
Model picker tests require KILO_API_KEY and KILO_ORG_ID to connect providers. In fork PR CI where secrets are unavailable, these tests were failing. Now they skip gracefully when credentials are missing.
|
e963daa fixes the cause of the test (linux) fails:
-> no providers connect -> model list is empty -> model picker has no list items -> 4 model picker tests fail added a check for these vars and skipping the 4 tests so the pipeline is green - if you fix the credentials issue they should run |
|
the other issue was that the two reason for changing was: symlinks don't work reliably on windows, and vite will fail reading them. changing into regular files with an export works |
|
all unit/integration tests pass again |
General Rebase/Conflict infoentire patch is 1 rule "normalize paths on win32 systems via Filesystem wrapper, where otherwise things will break"
|


Fixes #107
In reference to anomalyco/opencode#6763
Summary
This fix solves a whole series of bugs, resulting from filepath issues with backslashes occuring on Windows.
→ Solution: normalize all internal paths to forward slashes using centralized
FilesystemwrappersWhy this works: all win32 shells work with forward slash paths, this is de-facto industry standard (git, vscode, cmake, ... all use this pattern)
Observed issues
git rev-parse --show-toplevelreturnsE:/x/yforward slash format → worktree/directory mismatchpath.resolve(),path.relative(),realpathSync.native(),realpath(bash) break with git bash paths (various issues)/d/xwithin C drive (/c/) results in things likeC:\d\xcontainslogic not working → issues with permission system and external_directory/tmpinside git bash is something else outside of itspawn()is used with non-existing directoryTested with fix
bun testinpackages/opencode)C:\a\b,C:/a/b,/c/a/b,/cygdrive/c/a/b) are normalized intoC:/a/bformatENOENTerrors gone, permissions, relative paths, external directory, LSP)How to reproduce/test
Notes
A few more places would require normalization - but they're unused/dead code (should be removed)
Desktop likely having more issues, especially
bun.spawn()usage